You can actually avoid this whole if-statement if you do
module_name, _, function_name = function_namespace.rpartition(".")

I didnt know of .rpartition() on strings before, cool!

Rather than using globals() which would require that the module is already imported I think it might be better to use the importlib package.

I think you would write:

module = importlib.import_module(module_name)
fn = getattr(module, function_name)

The if loop is because if the module name is empty, i.e. the functions are included in the same module as the rest of the workflow for deriving variables (which they are at the moment), then the use of the importlib package doesn't work. Also, I didn't think it made sense to import the mllam_data_prep.ops.derived_variables since this is the module the call is being made from so that is why I added the globals() part for these 2 cases. For all other cases (the else statement) I am using the importlib package approach already.

However, since we're anyway splitting up the functions from the rest (in one way or another) this approach will need to change as well and then I will just go with the importlib package solution, as I have for the else statement part here.

Yes, I think that would be better too. So maybe the "load" part of load_and_subset_dataset should be split out into its own function so that we have load_input_dataset which is called once in create_dataset in https://github.com/ealerskans/mllam-data-prep/blob/feature/derive_forcings/mllam_data_prep/create_dataset.py#L137. And then rename the current load_and_subset_dataset function just subset_dataset. When calling load_input_dataset, maybe call what is returned ds_input and that can then be passed into both subset_dataset and derive_variables

Yes. I would put this constant into a global variable, something like CHUNK_MAX_SIZE_WARNING, but I would also put this "chunk size check into its own function somewhere outside of ops, maybe mllam_data_prep.chunking, or maybe mllam_data_prep.ops.chunking makes sense? I think this is something we might want to reuse :)

Rather than using globals() which would require that the module is already imported I think it might be better to use the importlib package.

I think you would write:

module = importlib.import_module(module_name)
fn = getattr(module, function_name)

This file is getting quite long. Should we maybe split the implementation of the individual functions into submodules? I also think this should maybe sit in mllam_data_prep.ops instead of in the root of the package.

We could maybe do something like

mllam_data_prep
   .ops
       .derive_variable
           .__init__  <-- you could import the functions that actually carry out the work from the modules mentioned below:
           .dispatcher <-- this would contain the function to derive single variable
           .physical_field <-- this could contain the toa field for example
           .time_components <-- the time components could go here

Does this makes sense? :)

I agree that it is getting long and it would be nice to split it up. The main reason for not adding it to mllam_data_prep.ops is that I don't know what "ops" stands for and what can/should be included here.

The idea and structure makes sense but the implementation is not very clear, probably because I am not very familiar with object oriented programming.

I guess it is mostly in connection with the other suggestion that we should loop over the derived variables in a call external to derive_variable (in create_dataset() I am assuming then, or somewhere else?) and what should be called from there.

I have also been a bit puzzeled about what the "ops" stands for. I think I have interpreted is as "operations", but not sure if this is correct. Do we document the meaning somewhere? Otherwise, we could maybe consider to rename it to make it more self-explainable (for another PR then).

Yes, it was meant to be shorthand for "operations", sorry... regarding splitting this file it isn't so much object-oriented programming, but more about just splitting functions into separate modules that have different purposes. So that would mean making a new subdirectory (called derived_variables) and creating a new python file (which will then each become a new submodule) and put the different functions in there.

But let's talk about it and I'll show you what I mean :)

Great that you've put this validation code all together. Instead of creating a new method called load_config I think it would be better to use the __post_init__() method (https://docs.python.org/3/library/dataclasses.html#dataclasses.__post_init__) that dataclasses can implement though. I would then make your validation code here be a function that isn't a member of the Config class but just put the global scope of this module (you could maybe call i validate_config?). Using __post_init__ will ensure this method is automatically called whatever the dataclass instance is initialised from (be it a yaml file, yaml formatted string, json or directly as a Config object)

if you make the change I suggested to use __post_init__ on the Config dataclass then you can still use from_yaml_file here which makes it clear we are just using that method as implemented by dataclasses-wizard :)

beautiful!

maybe we should rename this selected_variables? What do you think? It is a bit confusing to have both variables and derived_variables, maybe calling it selected_variables will make it clear that this part of the code deals with the ones that are "selected" from the input dataset

same here for the .from_yaml_file call :)

perfect!

It might be simpler (and more explicit which is often good) to change the config to explicitly call these features hour_of_day_cos and hour_of_day_sin, and that way each function only every returns one derived variable (which makes the rest of the code simpler I think)

you could split this into two functions or make cos or sin (as strings) an argument to this function say cyclically_encode_values(values, max_value, component='cos'). But inline might be better to make this more explicit (since it is only one line)

if the chunking is done here already (during load), why do we need to do it again later when deriving variables?

I don't quite understand the mocking, patching and tests in here :) but let's talk about it so I can learn!